Skip to content

Reworking fix of unstable GitConfig parsing#43

Merged
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:git-config-rework
Jun 2, 2026
Merged

Reworking fix of unstable GitConfig parsing#43
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:git-config-rework

Conversation

@unv-unv

@unv-unv unv-unv commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Avoid wrapping Ini with TreeMap since Ini is technically multi-map and this wrapping can cause data loss. Though this is unlikely the case for remotes, branches and urls, since section name in their case contains name parameter. But anyway better to sort parsed data. Also replacing static final nested config classes with records.

… TreeMap since Ini is technically multi-map and this wrapping can cause data loss. Though this is unlikely the case for remotes, branches and urls, since section name in their case contains name parameter. But anyway better to sort parsed data. Also replacing static final nested config classes with records.
@unv-unv

unv-unv commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @unv-unv's task in 4m 36s —— View job


PR Review: Reworking fix for unstable GitConfig parsing

  • Read the diff and full GitConfig.java context
  • Analyze correctness of Ini vs TreeMap change
  • Analyze TreeSet / Comparable record changes
  • Post inline review comments

Overall this is a clean, well-targeted refactor. Dropping the new TreeMap<>(...) wrapper and iterating the Ini directly is the right call, and converting the three config holders to records cuts a lot of boilerplate without changing behavior. I verified the removed accessors (getName(), getBean(), myName, …) aren't referenced anywhere else — the records are private, so the change is self-contained and compiles cleanly against the rest of the package.

Inline comments (2):

Notes

  • The switch from ArrayListTreeSet is effectively a sorting change: parsed entries are now ordered by name regardless of file order, which is a nice stability improvement. Because ini4j already merges same-named sections in entrySet(), the TreeSet's dedup-by-name never actually triggers in practice.
  • No tests cover this parsing path in the repo, so I couldn't add a regression test; behavior was reviewed statically.

Nothing blocking here — the single 🟡 item is a judgment call you've already reasoned about in the PR description. Looks good to merge. 👍
· git-config-rework

@unv-unv unv-unv changed the title Reworking fix ofr unstable GitConfig parsing Reworking fix of unstable GitConfig parsing Jun 2, 2026
) {
Collection<BranchConfig> configs = new ArrayList<>();
private static Collection<BranchConfig> parseTrackedInfos(@Nonnull Ini ini, @Nonnull ClassLoader classLoader) {
Collection<BranchConfig> configs = new TreeSet<>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@/home/runner/.claude/projects/-home-runner-work-consulo-git-consulo-git/memory/c1body.md

}

@Override
public int compareTo(Remote that) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@/home/runner/.claude/projects/-home-runner-work-consulo-git-consulo-git/memory/c2body.md

@VISTALL VISTALL merged commit 40d3f6a into consulo:master Jun 2, 2026
1 check passed
@unv-unv unv-unv deleted the git-config-rework branch June 2, 2026 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants